Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Delegation01 #1290

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Update Delegation01 #1290

merged 3 commits into from
Nov 28, 2023

Conversation

tgreenx
Copy link
Contributor

@tgreenx tgreenx commented Sep 12, 2023

Purpose

This PR updates Delegation01 by updating some message tags/ids to report name server names and addresses together instead of separately.

Context

Fixes zonemaster/zonemaster#1203

Changes

  • ENOUGH_IPV{4/6}_NS_CHILD
  • NOT_ENOUGH_IPV{4/6}_NS_CHILD
  • ENOUGH_IPV{4/6}_NS_DEL
  • NOT_ENOUGH_IPV{4/6}_NS_DEL
  • ENOUGH_NS_{CHILD/DEL}
  • NOT_ENOUGH_NS_{CHILD/DEL}

How to test this PR

Unit tests should pass.

Also:

$ zonemaster-cli --show-testcase --test delegation/delegation01 --level info ripe.net
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     UNSPECIFIED    Using version v4.7.2 of the Zonemaster engine.
   0.73 INFO     DELEGATION01   Parent lists enough (5) nameservers. Lower limit set to 2. Name servers: manus.authdns.ripe.net;ns3.afrinic.net;ns3.lacnic.net;ns4.apnic.net;rirns.arin.net
   2.88 INFO     DELEGATION01   Child lists enough (5) nameservers. Lower limit set to 2. Name servers: manus.authdns.ripe.net;ns3.afrinic.net;ns3.lacnic.net;ns4.apnic.net;rirns.arin.net
   2.88 INFO     DELEGATION01   Child lists enough (5) nameservers that resolve to IPv4 addresses. Lower limit set to 2. Name servers: manus.authdns.ripe.net/193.0.9.7;ns3.afrinic.net/204.61.216.100;ns3.lacnic.net/200.3.13.14;ns4.apnic.net/202.12.31.53;rirns.arin.net/199.253.249.53
   2.88 INFO     DELEGATION01   Child lists enough (5) nameservers that resolve to IPv6 addresses. Lower limit set to 2. Name servers: manus.authdns.ripe.net/2001:67c:e0::7;ns3.afrinic.net/2001:500:14:6100:ad::1;ns3.lacnic.net/2001:13c7:7002:3000::14;ns4.apnic.net/2001:dd8:12::53;rirns.arin.net/2620:38:2000::53
   2.92 INFO     DELEGATION01   Delegation lists enough (5) nameservers that resolve to IPv4 addresses. Lower limit set to 2. Name servers: manus.authdns.ripe.net/193.0.9.7;ns3.afrinic.net/204.61.216.100;ns3.lacnic.net/200.3.13.14;ns4.apnic.net/202.12.31.53;rirns.arin.net/199.253.249.53
   2.92 INFO     DELEGATION01   Delegation lists enough (5) nameservers that resolve to IPv6 addresses. Lower limit set to 2. Name servers: manus.authdns.ripe.net/2001:67c:e0::7;ns3.afrinic.net/2001:500:14:6100:ad::1;ns3.lacnic.net/2001:13c7:7002:3000::14;ns4.apnic.net/2001:dd8:12::53;rirns.arin.net/2620:38:2000::53

And :

$ zonemaster-cli --show-testcase --test delegation/delegation01 --level info --ns ns1.m.delegation01.exempelvis.se/46.21.97.97 --ns ns1.m.delegation01.exempelvis.se/194.18.226.122 m.delegation01.exempelvis.se
Seconds Level    Testcase       Message
======= ======== ============== =======
   0.00 INFO     UNSPECIFIED    Using version v4.7.2 of the Zonemaster engine.
   0.09 ERROR    DELEGATION01   Parent does not list enough (1) nameservers. Lower limit set to 2. Name servers: ns1.m.delegation01.exempelvis.se
   0.33 ERROR    DELEGATION01   Child does not list enough (1) nameservers. Lower limit set to 2. Name servers: ns1.m.delegation01.exempelvis.se
   0.34 ERROR    DELEGATION01   Child does not list enough (1) nameservers that resolve to IPv4 addresses. Lower limit set to 2. Name servers: ns1.m.delegation01.exempelvis.se/194.18.226.122;ns1.m.delegation01.exempelvis.se/46.21.97.97
   0.34 NOTICE   DELEGATION01   Child lists no nameserver that resolves to an IPv6 address. If any were present, the minimum allowed would be 2.
   0.34 ERROR    DELEGATION01   Delegation does not list enough (1) nameservers that resolve to IPv4 addresses. Lower limit set to 2. Name servers: ns1.m.delegation01.exempelvis.se/194.18.226.122;ns1.m.delegation01.exempelvis.se/46.21.97.97
   0.34 NOTICE   DELEGATION01   Delegation lists no nameserver that resolves to an IPv6 address. If any were present, the minimum allowed would be 2.

Note that one side effect is that when there is a single name server with several IP addresses, the message might not be obvious at first (counts one name server name, but shows two):

2.14 ERROR DELEGATION01 Child does not list enough (1) nameservers (ns1.m.delegation01.exempelvis.se/194.18.226.122;ns1.m.delegation01.exempelvis.se/46.21.97.97) that resolve to IPv4 addresses. Lower limit set to 2.

@tgreenx tgreenx added A-TestCase Area: Test case specification or implementation of test case V-Patch Versioning: The change gives an update of patch in version. labels Sep 12, 2023
@tgreenx tgreenx added this to the v2023.2 milestone Sep 12, 2023
Update some message ids to report name server names and addresses together instead of separately
@marc-vanderwal
Copy link
Contributor

I think it would be nice if names resolving to multiple IP addresses could be represented in a less verbose way, and with the IP addresses sorted according to their 32- or 128-bit integer values. For example, instead of having:

ns1.m.delegation01.exempelvis.se/194.18.226.122;ns1.m.delegation01.exempelvis.se/46.21.97.97

use this representation:

ns1.m.delegation01.exempelvis.se/46.21.97.97,194.18.226.122

Otherwise, name server names and IP tuples would be excessively hard to read. Especially when IPv6 is involved.

@matsduf
Copy link
Contributor

matsduf commented Sep 18, 2023

I think it would be nice if names resolving to multiple IP addresses could be represented in a less verbose way, and with the IP addresses sorted according to their 32- or 128-bit integer values. For example, instead of having:

I think it is uncommon to have multiple IP addresses for the same name server name. The example that you give is a test zone. We should not change anything based on that test zone.

If this is to be changed, then it should be specified first so that we do not get an ad-hoc format for one test case.

I agree that it can be hard to read when there is a long list of name servers. For GUI I have created an issue with a suggestion to make it easier to digest: zonemaster/zonemaster-gui#315

marc-vanderwal
marc-vanderwal previously approved these changes Sep 18, 2023
lib/Zonemaster/Engine/Test/Delegation.pm Outdated Show resolved Hide resolved
Move name server list to the end of the message ids
lib/Zonemaster/Engine/Test/Delegation.pm Outdated Show resolved Hide resolved
lib/Zonemaster/Engine/Test/Delegation.pm Outdated Show resolved Hide resolved
Update message ids ENOUGH_NS_DEL and NOT_ENOUGH_NS_DEL
@tgreenx tgreenx requested a review from matsduf October 10, 2023 14:18
@tgreenx
Copy link
Contributor Author

tgreenx commented Nov 2, 2023

@matsduf please re-review

@tgreenx tgreenx merged commit df992af into zonemaster:develop Nov 28, 2023
3 checks passed
@tgreenx tgreenx deleted the update-delegation01 branch November 28, 2023 14:47
@ghost
Copy link

ghost commented Jan 10, 2024

v2023.2 release testing

Tested using the "How to test section" and works as expected.

@ghost ghost added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jan 10, 2024
marc-vanderwal added a commit to marc-vanderwal/zonemaster-backend that referenced this pull request Jan 23, 2024
Since the last release, a few message IDs were changed in
Delegation01 (see zonemaster/zonemaster-engine#1290). Instead of having
separate {nsname_list} and {ns_ip_list} arguments, these messages now
use a merged {ns_list}.

After the database migration, looking up results for older tests could
cause translated messages to be returned where the {ns_list} placeholder
was not substituted, because nothing was done to migrate the nsname_list
and ns_ip_list arguments to ns_list.

This commit ensures that the messages in question are properly migrated
as well.

Please note, however, that the name server names and IP addresses are
not guaranteed to match (see zonemaster/zonemaster#1203). There is
unfortunately no way to recover the correct name server name to IP
mapping, so the migration code just assumes that the two lists can be
zipped together.
marc-vanderwal added a commit to mattias-p/zonemaster-backend that referenced this pull request Jan 23, 2024
Since the last release, a few message IDs were changed in
Delegation01 (see zonemaster/zonemaster-engine#1290). Instead of having
separate {nsname_list} and {ns_ip_list} arguments, these messages now
use a merged {ns_list}.

After the database migration, looking up results for older tests could
cause translated messages to be returned where the {ns_list} placeholder
was not substituted, because nothing was done to migrate the nsname_list
and ns_ip_list arguments to ns_list.

This commit ensures that the messages in question are properly migrated
as well, for PostgreSQL, MySQL/MariaDB and SQLite.

Please note, however, that the name server names and IP addresses are
not guaranteed to match (see zonemaster/zonemaster#1203). There is
unfortunately no way to recover the correct name server name to IP
mapping, so the migration code just assumes that the two lists can be
zipped together.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-TestCase Area: Test case specification or implementation of test case S-ReleaseTested Status: The PR has been successfully tested in release testing V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Child IP addresses not listed in same order as nameservers
3 participants